[ty] Add initial implementation of goto definition for loads of local names #19123

Gankra · 2025-07-03T15:15:09Z

No description provided.

Gankra · 2025-07-03T18:37:28Z

crates/ty_python_semantic/src/semantic_model.rs

+                let index = semantic_index(model.db, model.file);
+                let file_scope = index.expression_scope_id(*self);
+                let scope = file_scope.to_scope_id(model.db, model.file);
+                let use_def = index.use_def_map(file_scope);
+                let use_id = self.scoped_use_id(model.db, scope);
+
+                Some(
+                    use_def
+                        .bindings_at_use(use_id)
+                        .filter_map(|binding| binding.binding.definition())
+                        .collect(),
+                )


This is basically the happy path of TypeInferenceBuilder::infer_place_load. It's extremely limited because it can't walk into parent scopes, and because it never engages type inference, it can't reason about "oh this is MyClass.myfield".

I feel like the more Correct solution we want here is to augment infer_place_load to also yield Definition info, and then run the whole TypeInferenceBuilder machinery just like inferred_type does.

Gankra · 2025-07-03T18:37:46Z

crates/ty_python_semantic/src/types/call/bind.rs

@@ -2667,7 +2667,7 @@ impl<'db> BindingError<'db> {
                if let Some(builder) = context.report_lint(&MISSING_ARGUMENT, node) {
                    let s = if parameters.0.len() == 1 { "" } else { "s" };
                    let mut diag = builder.into_diagnostic(format_args!(
-                        "No argument{s} provided for required parameter{s} {parameters}{}",
+                        "No argument{s} provided for required parameterzz{s} {parameters}{}",


Gankra · 2025-07-03T18:38:54Z

crates/ty_server/src/server/api/requests/goto_definition.rs

+pub(crate) struct GotoDefinitionRequestHandler;
+
+impl RequestHandler for GotoDefinitionRequestHandler {
+    type RequestType = GotoDefinition;
+}


Everything in this file is a copy of goto_type_definition but changed to goto_definition

Gankra · 2025-07-03T18:41:20Z

crates/ty_ide/src/goto.rs

+        assert_snapshot!(test.goto_definition(), @r"
+        info[goto-type-definition]: Type definition
+         --> main.py:4:13
+          |
+        2 |             ab = 1
+        3 |             ab = 2
+        4 |             ab = 3
+          |             ^^
+        5 |             print(ab)


This is the first non-trivial property of the current implementation: the "definitions" we yield are the bindings that dominate the load. This is a useful answer but obviously not the only possible one.

Gankra · 2025-07-03T18:42:19Z

crates/ty_ide/src/goto.rs

+        2 |             ab = 1
+        3 |             if cond:
+        4 |                 ab = 2
+          |                 ^^
+        5 |             print(ab)


(in this case we point to both ab = 1 (above) and ab = 2 as both bindings can affect the load)

Gankra · 2025-07-03T18:43:25Z

crates/ty_ide/src/goto.rs

+            ab: int
+            print(a<CURSOR>b)
+        "#,
+        );
+
+        assert_snapshot!(test.goto_definition(), @"No definitions found");


In this case there's no bindings for ab so we fail to yield anything (suboptimal).

This would be part of goto declarations instead :)

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_declaration

Gankra · 2025-07-03T18:46:55Z

crates/ty_ide/src/goto.rs

+            a<CURSOR>b += 2
+            print(ab)
+        "#,
+        );
+
+        assert_snapshot!(test.goto_definition(), @"No goto target found");


Any kind of Store completely falls over right now -- it seems like most of the code gives up when it sees Store, even if it's also a Load, but maybe it's handled by different paths? (in the playground ab += 2 does seem to support getting the type of ab)

Yeah, augmented assignments are both LOAD and STORE.

Gankra · 2025-07-03T18:48:15Z

crates/ty_ide/src/goto.rs

+        2 |             class AB(val: int):
+          |                   ^^
+        3 |                 self.myval = val
+          |
+        info: Source
+         --> main.py:5:17
+          |
+        3 |                 self.myval = val
+        4 |
+        5 |             x = AB(5)
+          |                 ^^


Note that this is only working because the class is defined in the same scope. If x = AB(5) was in a function it would fail to resolve, because we don't look into parent scopes. yet.

Gankra · 2025-07-03T18:48:52Z

crates/ty_ide/src/goto.rs

+            x = AB(5)
+            print(x.my<CURSOR>val)
+        "#,
+        );
+
+        assert_snapshot!(test.goto_definition(), @"No goto target found");


This one is hopeless without type info.

Gankra · 2025-07-03T18:49:28Z

crates/ty_ide/src/goto.rs

+                RED = "red"
+                BLUE = "blue"
+
+            x = AB.RE<CURSOR>D


Similarly hopeless without type info

Gankra · 2025-07-03T18:50:31Z

crates/ty_ide/src/goto.rs

+            ab = 1
+            def myfunc():
+                global ab
+                print(a<CURSOR>b)
+        "#,
+        );
+
+        assert_snapshot!(test.goto_definition(), @"No definitions found");


Globals of course fail here without processing of parent scopes.

github-actions · 2025-07-03T23:24:37Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

dhruvmanila

Thanks for taking this on! This seems like a good start but I'm a bit hesitant on shipping this based on the limitation that this only works if both the definition and usage are in the same scope.

The reason this limitation exists for goto definition and not goto type definition is because the former is only looking at the scope of the expression where it's being used while the latter is performing the type inference and fetching the list of definitions from Type instead (Type::definition). I think you're most likely aware of this already.

The reason it's fine for goto type definition is because we actually need the type definition which would either be in the stub file (if present) or the runtime file. This is the behavior of type inference in ty so it falls out naturally in the LSP.

For goto definition, I think we'll need something else. One way to solve this would be the following which is a bit high level:

This is something that Eric mentioned -- create a source mapping between the definitions in runtime and stub file
Use the same path as goto type definition but use the above mapping to get the corresponding runtime definition instead of the type definition

But to decrease the scope, we could probably ship a limited version which performs the same steps as goto type definition but filters out all the targets that would lead to a stub file and only return the ones that are present in the runtime files. This does mean that goto definition on any builtin symbols or the ones originating in the standard library wouldn't work because they all go to the stub files present in typeshed.

dhruvmanila · 2025-07-08T11:33:26Z

crates/ty_server/src/server/api/requests/goto_definition.rs

+        if snapshot
+            .resolved_client_capabilities()
+            .type_definition_link_support


This should use the linkSupport value from the DefinitionClientCapabilities instead: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_definition

You'll need to add a new definition_link_support field in ResolvedClientCapabilities which takes it's value from document.definition.link_support. Refer to how type_definition_link_support is being computed.

Hmm what distinguishes this from goto_type_definition, which has the same impl? (or is this a "also fix goto_type_definition"?)

These are the client capabilities, so if a client has link support for goto type definition but not for goto definition (for whatever reason), the server needs to respect that. These capabilities are provided to the server during initialization where we make note of all the things that the client supports through which the server needs to make certain decisions. This is one of them.

dhruvmanila · 2025-07-08T11:38:08Z

crates/ty_python_semantic/src/semantic_model.rs

+                    )
+                }
+                ExprContext::Store => None,
+                ExprContext::Del => None,


I think we can include ExprContext::Del here as the following should go to the definition of x:

def _(): x = 1 del x<CURSOR>

dhruvmanila · 2025-07-08T11:39:21Z

crates/ty_python_semantic/src/semantic_model.rs

+                            .collect(),
+                    )
+                }
+                ExprContext::Store => None,


This might a bit tricky because the definition that corresponds to this expression itself should also be included in the list of definitions. For example, here both definitions should be included:

def _(): x = 1 x<CURSOR> = 2

dhruvmanila · 2025-07-08T11:51:46Z

crates/ty_ide/src/goto.rs

+            a<CURSOR>b += 2
+            print(ab)
+        "#,
+        );
+
+        assert_snapshot!(test.goto_definition(), @"No goto target found");


Yeah, augmented assignments are both LOAD and STORE.

dhruvmanila · 2025-07-08T11:52:48Z

crates/ty_python_semantic/src/semantic_model.rs

+                ExprContext::Load => {
+                    let index = semantic_index(model.db, model.file);
+                    let file_scope = index.expression_scope_id(*self);
+                    let scope = file_scope.to_scope_id(model.db, model.file);
+                    let use_def = index.use_def_map(file_scope);
+                    let use_id = self.scoped_use_id(model.db, scope);
+
+                    Some(
+                        use_def
+                            .bindings_at_use(use_id)
+                            .filter_map(|binding| binding.binding.definition())
+                            .collect(),
+                    )
+                }


Oh hmm, I see what you mean based on what you mentioned on Discord. So, this implementation would be limited to only provide definitions if they're defined in the current scope, right? The following doesn't work:

x = 1 def _(): x<CURSOR>

Yes correct.

dhruvmanila · 2025-07-08T11:55:48Z

crates/ty_ide/src/goto.rs

+
+    Some(RangedValue {
+        range: FileRange::new(file, goto_target.range()),
+        value: NavigationTargets::unique(targets),


Why do we require to call unique here?

For whatever reason it's the constructor that takes a list (the name was concerning, but, it works?)

The idea behind using unique here is that, at least for go to type definition, we want to avoid showing the same definition twice. E.g. if you have a Class<str> | Class<int> type. We only want to list one target because both those type point to the generic Class<T>

Thanks, I think this makes sense for goto definitions as well.

dhruvmanila · 2025-07-08T12:18:30Z

crates/ty_ide/src/goto.rs

@@ -29,6 +30,35 @@ pub fn goto_type_definition(
    })
 }

+pub fn goto_definition(


I think we should split this into two files: goto_type_definition.rs and goto_definition.rs to isolate the logic and the tests. I'm not exactly sure where should the shared logic reside in, maybe the goto.rs could be used for that but that sounds a bit confusing.

dhruvmanila · 2025-07-08T12:20:16Z

crates/ty_ide/src/goto.rs

+            let source = targets.range;
+            self.render_diagnostics(
+                targets
+                    .into_iter()
+                    .map(|target| GotoTypeDefinitionDiagnostic::new(source, &target)),
+            )


We might want to create a generic GotoDiagnostic which takes in an enum that specifies which kind it is: GotoKind::Definition, GotoKind::TypeDefinition, GotoKind::Declaration, etc.

Gankra added server Related to the LSP server ty Multi-file analysis & type inference labels Jul 3, 2025

Gankra commented Jul 3, 2025

View reviewed changes

crates/ty_ide/src/goto.rs

RED = "red"

BLUE = "blue"

x = AB.RE<CURSOR>D

Copy link

Contributor Author

Gankra Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly hopeless without type info

Gankra commented Jul 3, 2025

View reviewed changes

Gankra added 3 commits July 3, 2025 19:20

Add initial implementation of goto definition for loads of local names

d7b7b83

add many tests

1bee527

fixup

5cc6762

Gankra force-pushed the gankra/gotodef branch from ec1ff02 to 5cc6762 Compare July 3, 2025 23:20

fmt

97af9d8

dhruvmanila reviewed Jul 8, 2025

View reviewed changes

[ty] Add initial implementation of goto definition for loads of local names #19123

Are you sure you want to change the base?

[ty] Add initial implementation of goto definition for loads of local names #19123

Uh oh!

Conversation

Gankra commented Jul 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

dhruvmanila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhruvmanila Jul 8, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading

`mypy_primer` results

dhruvmanila left a comment •

edited

Loading